Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[CS2103T-W12-4] IchiFund #122

Open
wants to merge 897 commits into
base: master
Choose a base branch
from

Conversation

weiyang13
Copy link

No description provided.

Copy link

@ChenJiehan318 ChenJiehan318 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For user stories, the type of user is not specific enough. Instead of one type of user, maybe it will be better if there are more types of users.


[appendix]
== Non Functional Requirements

. Should work on any <<mainstream-os,mainstream OS>> as long as it has Java `11` or above installed.
. Should be able to hold up to 1000 persons without a noticeable sluggishness in performance for typical usage.
. Should be able to hold up to 1000 transactions without a noticeable sluggishness in performance for typical usage.
. A user with above average typing speed for regular English text (i.e. not code, not system admin commands) should be able to accomplish most of the tasks faster using commands than using the mouse.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should there be a well-defined benchmark to meet? (above average typing speed is subjective)

@@ -79,14 +80,16 @@ image::UiClassDiagram.png[]

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Diagrams are really neat and matches your project (from what I can tell)

This feature allows the user to add a repeater in IchiFund. Adding a repeater also creates the transactions associated with the added repeater.

==== Implementation
The `addrep` command is facilitated by the Logic and Model components of the application. Given below is an example usage scenario of how `addrep` behaves at each step.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would be neater if you included a diagram.

@@ -358,25 +699,24 @@ _{More to be added}_
[[mainstream-os]] Mainstream OS::

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could add terms like 'repeater' to the glossary as people might be unsure of what it means.


*MSS*

1. User requests to mark a lone as paid off.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could use more extensions

==== Adding Transactions
===== Implementation
===== Design Considerations
// end::transadd[]

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe can consider adding more activity diagrams.

Comment on lines +166 to +180
==== Tasks
Some models in IchiFund must be refreshed after a command is executed.
For instance, when a new `Transaction` is added, all `Budget` must be recomputed.
`Task` can be used to facilitate such updates.

===== Implementation
This feature is managed by `TaskManager`.
The role of `TaskManager` is to maintain a list of all active `Task`.

The `LogicManager` holds an instance of the `TaskManager`.
When the `LogicManager#execute()` is called, the following chain of operations occurs:

1. After `Command#execute()` is completed, `TaskManager#executeAll()` is called.

image::TaskCode.png[]

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An example usage scenario could be useful in making the purpose of Tasks clearer.

Copy link

@Russell-Loh-NUS Russell-Loh-NUS left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall great job on the developer guide!

=== Repeaters

// tag::repeateradd[]
=== Adding Repeater : `addrep`

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will be better if this is a smaller heading as compared to the 'Repeaters' header.


This process is further illustrated in the following sequence diagram:

.Sequence Diagram for `add` Command under Budget Tab

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Diagram is neat and symbols are clear with good use of lifeline ending.

// end::repeateradd[]

// tag::repeaterdelete[]
=== Deleting Repeater : `delrep`

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably will be better to include a sequence diagram.


// tag::tasks[]
==== Tasks
Some models in IchiFund must be refreshed after a command is executed.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably will be better to include the models that will be affected


The `UI` component uses JavaFx UI framework. The layout of these UI parts are defined in matching `.fxml` files that are in the `src/main/resources/view` folder. For example, the layout of the link:{repoURL}/src/main/java/seedu/address/ui/MainWindow.java[`MainWindow`] is specified in link:{repoURL}/src/main/resources/view/MainWindow.fxml[`MainWindow.fxml`]

The `UI` component,

* Executes user commands using the `Logic` component.
* Listens for changes to `Model` data so that the UI can be updated with the modified data.
* Listens for changes to the index of the `FeatureParser` used by the `Logic` component to change tabs.
* Sets the `FeatureParser` used by the `Logic` component when the user manually changes tabs.

[[Design-Logic]]
=== Logic component

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Diagram is similar to the 2.1 Architecture diagram. Probably will be better to include a reference instead of repeating the diagram. Arrows can be bigger for the association and dependency arrows.

alages97 pushed a commit to alages97/main that referenced this pull request Nov 1, 2019
LeeYiyuan and others added 30 commits November 11, 2019 22:25
…Alaete/main into update-developer-guide-and-ppp-for-v1.4
# Conflicts:
#	docs/DeveloperGuide.adoc
Add test cases in DG; other team tasks
Fix user guide for v1.4 and implement UpdateReportTask
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.